Skip to content

GH-939: Remove reflection for gRPC buffers #954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
xborder wants to merge 6 commits into
base: main
Choose a base branch
from
Open

GH-939: Remove reflection for gRPC buffers #954

xborder wants to merge 6 commits into from
+347 −105

Conversation

@xborder
Copy link
Contributor

@xborder xborder commented Jan 11, 2026

What's Changed

  • The branch removes the previous reflection-based access to internal gRPC buffers and replaces it with supported gRPC interfaces (Detachable, HasByteBuffer) to safely transfer ownership of direct buffers to Arrow without copying.
  • gRPC direct ByteBuffer streams are detached and wrapped as ForeignAllocation for zero‑copy reads
  • Fall back to safe buffer copies when gRPC does not expose a direct ByteBuffer or data is fragmented
  • Added test coverage

Closes #939.

@github-actions

This comment has been minimized.

Sign in to view
@xborder xborder changed the title GH-939: Remove relection GH-939: Remove reflection for gRPC buffers Jan 11, 2026
@xborder xborder marked this pull request as draft January 11, 2026 00:47
@lidavidm lidavidm added the enhancement PRs that add or improve features. label Jan 11, 2026
@xborder xborder marked this pull request as ready for review January 11, 2026 15:02
@github-actions github-actions bot added this to the 19.0.0 milestone Jan 11, 2026
lidavidm
lidavidm reviewed Jan 12, 2026
View reviewed changes
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java Outdated
Comment on lines 463 to 466
if (detachedByteBuffer == null || !detachedByteBuffer.isDirect()) {
closeQuietly(detachedStream);
return null;
}
Copy link
Member

@lidavidm lidavidm Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we detached the stream, if we give up here, aren't we erroneously going to discard the actual data? My understanding of Detachable is that the original stream is now invalid, right?

Copy link
Contributor Author

@xborder xborder Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. Actually this is redundant, the isDirect is already checked at the beginning of the method so I just removed it

lidavidm
lidavidm reviewed Jan 14, 2026
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All builds fail:

Error:  Non-test scoped test only dependencies found:
Error:     io.grpc:grpc-core:jar:1.78.0:compile

lidavidm
lidavidm approved these changes Jan 15, 2026
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved CI

@lidavidm
Copy link
Member

lidavidm commented Jan 15, 2026

CC @ejona86 in case you do want to look over this

@lidavidm
Copy link
Member

lidavidm commented Jan 15, 2026

@xborder it seems this doesn't actually work...

@xborder
Copy link
Contributor Author

xborder commented Jan 15, 2026 •
edited
Loading

@lidavidm Seems like a chicken and the egg problem. grpc-core can't be a compile dependency because it is detected as unused by CI although it is still required in runtime (AddWritableBuffer still needs it for reflection).
Marking it with a runtime scope causes the tests I created to fail because they require the dependency in compile time.

I've changed the tests to use a mock for the dependency it had on grpc-core. This seemed to have solved the issues from the two previous runs but probably diminishes the relevance of these tests

@jbonofre
Copy link
Member

jbonofre commented Jan 15, 2026

@xborder thanks for the update. Let me take a look.

@xborder
Copy link
Contributor Author

xborder commented Jan 15, 2026 •
edited
Loading

@lidavidm I think there's something I overlooked. frame() is iterating over the stream, if I detach it when reading APP_METADATA_TAG it invalidates the same stream it is iterating. Let me rework on this and I'll keep you posted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidm lidavidm lidavidm approved these changes

@laurentgo laurentgo Awaiting requested review from laurentgo laurentgo is a code owner

@wgtmac wgtmac Awaiting requested review from wgtmac wgtmac is a code owner

@jbonofre jbonofre Awaiting requested review from jbonofre jbonofre is a code owner

Assignees

No one assigned

Labels

enhancement PRs that add or improve features.

Projects

None yet

Milestone

19.0.0

Development

Successfully merging this pull request may close these issues.

[FlightRPC] Remove sketchy gRPC reflection code

3 participants

@xborder @lidavidm @jbonofre